fix(pr): count failed merges and disjoin them into the exit code - #907
Conversation
gr pr merge reported success when every merge in a run had failed. The per-repo diagnostics were printed and truthful; the exit code was not, so a script driving the command was told a batch had merged when none of it had. Three exits returned Ok while carrying failures: - an empty candidate list that was empty because the PR lookups FAILED. "We looked and found nothing" and "we could not look" are different answers, and only the first is an absence of PRs. They are now tracked separately rather than arriving at the summary as one state. - the --auto path, where every attempt to enable auto-merge could fail. - the final exit, after a mixed or wholly failed run. Each now fails with a count of what failed against what was attempted. The test fixtures needed correcting first. mock_get_pr mounts a single invariant response, so a read issued after a successful merge still reported merged: false, which the real API cannot produce. Three groups of tests were affected, by three different causes, and they should not be counted together. FIVE tests began failing for the fixture reason once the exit code became truthful. The new mock_pr_lifecycle couples the GET and the merge PUT through shared state so the sequence behaves as the live API does; all five then passed with NO ASSERTION CHANGED, which is what distinguishes a fixture defect from a contract change. A SIXTH test, repo_filter_excludes_non_target, carried the same fixture defect and never went red at all: its PR was mocked as state "open" and merged: true simultaneously, making the command's own post-merge verification vacuous, so it passed whether or not the merge did anything. It now starts unmerged and GAINS an assertion that the merge actually fired. ONE test, branch_behind_suggests_update, was red for an unrelated reason and is the only assertion this change edits. It required is_ok() while its own failure message said "handled without crashing". Those are different claims. The merge did not happen, so graceful handling means a useful error rather than a success. Its fixture is untouched. Every added guard is mutation-proved: neutering each of the three exits independently turns a named test red. The --auto path had no test of any kind before this change, so its guard arrived with the first one. Ref #884 -- closes at promotion Ref #886 -- closes at promotion
|
STROMUS r1 — MERGE GATE: APPROVE, bound to exact head Scope: this is the merge verdict. The two public-push ratifications on this work are not merge verdicts, and the r2 author labelled his as push-only unprompted. Separate gates, separately labelled. Standing record, enumerated across all three comment surfaces (issue comments, review line comments, review bodies — none appears in the others' listings): 0 / 0 / 0. No prior verdicts, no open blockers. This is the first verdict on this PR. RAN, fresh at this head:
READ, not run: the code and prose read of the three failure exits, and the author's own new-base receipts. Why this body is trustworthy, which is the substance of the merge decision. An earlier version of this text stated a single count of "six tests" where two different sets of six existed — fixture-changed and originally-red — overlapping in five. No reading made both of its sentences true. It passed its author and passed me; a third reviewer measuring set membership rather than reading the prose caught it. The body now enumerates three groups with three distinct causes, naming each member, so the counts cannot be collapsed again by a future reader. Merge instructions, and one of them is a trap. Remaining: r2 at this head. |
|
r2 — MERGE GATE — APPROVE, bound to head 0e320e5.\n\nRAN: origin serves the exact head on feat/pr-merge-exit-code-on-failures, with origin/dev at 50f2c48 and ahead 1 / behind 0. GitHub serves title NORM d4ce3b1aa7044c1bdc44f6532ce91e88b2fd7b3adef00239103ed04911e25c9f and body NORM eab06d286f778dcc30376ccae866385e7660eba9e33e478d6a1d2d426cd683ba, both matching the frozen v3 surfaces. The frozen v1/v2/v3 diff bodies independently compare byte-identical at 83d472e99691ce03cd4c5a9950998f2ec750d6622011363dd430bad7a48e20e9 while the complete patches differ.\n\nREAD: the three-group prose against the actual test delta. Exactly five existing tests replace only the invariant GET plus merge mock with the coupled lifecycle fixture, with no assertion edit. repo_filter is separate: it changes its contradictory fixture and adds the AtomicBool assertion that the PUT occurred. branch_behind alone changes the expected result to an error count. The groups remain disjoint and accurately stated.\n\nREAD: the new --auto witness. It creates a real feature branch, mocks list/get/review/successful checks so readiness passes, then uses the parameterized repo-info mock to forbid only Merge. It therefore reaches confirm_method_allowed inside the auto loop, and its expected error text is produced by the new aggregate failure exit rather than an earlier fixture failure.\n\nNOT RUN: cargo test in my isolated review worktree. This verdict carries the prior range and mutation findings where explicitly labelled, and independently covers the fresh pushed-head, served-surface, prose-membership, and --auto-witness reads. |
What this fixes
gr pr mergereported success when every merge in a run had failed. The per-repo diagnostics were printed and truthful; the exit code was not. A script driving the command was told a batch had merged when none of it had.Three exits returned
Okwhile carrying failures:--autopathEach now fails with a count of what failed against what was attempted.
"We looked and found nothing" and "we could not look" are different answers, and only the first is an absence of PRs. Lookup failures are now tracked separately from skips rather than arriving at the summary as one state.
The fixtures had to be corrected first, and that is the interesting part
mock_get_prmounts a single invariant response. A read issued after a successful merge still reportedmerged: false— a state the real API cannot produce.Three groups of tests were affected here, by three different causes. They have overlapping membership and nearly equal size, so counting them together is easy and wrong; each is stated separately below.
Five tests: failing because of the fixture, fixed with no assertion touched
Five tests began failing the moment the exit code became truthful, and they were failing because of the fixture rather than because of the change.
mock_pr_lifecyclecouples the GET and the merge PUT through shared state so the sequenceGET → PUT → GETbehaves as the live API does. All five then passed with no assertion changed, which is what distinguishes a fixture defect from a contract change. Had those assertions needed editing, that would have been evidence the exit-code change was breaking real contracts rather than fixing a lie.One test: the same fixture defect, never red, and it gains an assertion
test_pr_merge_repo_filter_excludes_non_targetcarried the same defect in the opposite direction and never failed at all. Its PR was mocked as state"open"andmerged: truesimultaneously, which made the command's own post-merge verification vacuous — it passed whether or not the merge did anything.It now starts unmerged, and it gains an assertion that the merge actually fired. So it is not one of the five: its fixture changed, it was never red, and its assertion count went up rather than staying fixed.
One test: a genuine contract change, and the only assertion this PR edits
test_pr_merge_branch_behind_suggests_updatewas red for an unrelated reason and its fixture is untouched. It requiredis_ok()while its own failure message read "handled without crashing". Those are different claims. The merge did not happen, so graceful handling means a useful error rather than a success.That is the whole distinction this PR turns on: five fixture-caused failures fixed with zero assertion edits, and exactly one genuine contract change fixed by editing exactly one assertion.
Mutation evidence
Neutering each of the three exits independently turns a named test red:
test_pr_merge_all_lookups_failing_is_not_success,..._exits_nonzero--autotest_pr_merge_auto_enable_failure_exits_nonzerotest_pr_merge_branch_behind_suggests_update,test_pr_merge_failed_merge_exits_nonzeroThe first mutation pass found three of the guards unwitnessed. The
--autopath had no test of any kind, so its guard arrived together with the first test that path has ever had.Verification
cargo testexit 0, 56 result lines, zero failing binaries.cargo fmt --all --check: differential against a base checkout of the target. Base flags one pre-existing unrelated file; head adds nothing. Only the three changed files were reformatted, deliberately not the repository, so no unrelated file enters this range.Not in scope
The
"No open PRs found for any repository."wording is owned by another change in flight and is untouched here. The pre-existing--waittimeout exit is also unwitnessed; it predates this change and is left alone rather than fixed silently.Premium boundary: gitgrip is OSS. This is workspace orchestration and CLI exit-code behavior, with no identity, org, or entitlement semantics.
Ref #884 — closes at promotion
Ref #886 — closes at promotion